Skip to content

[SPARK-58561][INFRA] Support backporting a PR merged into a non-default branch - #57764

Open
uros-b wants to merge 4 commits into
apache:masterfrom
uros-b:merge-script-backport-nondefault
Open

[SPARK-58561][INFRA] Support backporting a PR merged into a non-default branch#57764
uros-b wants to merge 4 commits into
apache:masterfrom
uros-b:merge-script-backport-nondefault

Conversation

@uros-b

@uros-b uros-b commented Aug 4, 2026

Copy link
Copy Markdown
Member

What changes were proposed in this pull request?

Make dev/merge_spark_pr.py able to backport a PR that was merged into a non-default branch, and default its cherry-pick prompt to a branch that does not already have the change.

Backport mode is entered when the script finds the commit that merged an already-closed PR. That lookup previously read the merge commit off the PR's closed event:

merge_commits = [e for e in pr_events if e["event"] == "closed" and e["commit_id"] is not None]

GitHub only attributes a commit to the closed event when that commit lands on the default branch, because the Closes #N keyword in the commit message is what closes the PR and the keyword is honored only there. A PR merged into any other branch -- e.g. one opened against a rolling branch-M.x -- is instead closed by this script through the API (close_pr), and that closed event carries no commit. The merge then survives only as a referenced event, so merge_commits came up empty, backport mode never engaged, and the script fell through to the normal merge path -- offering to merge the PR a second time.

This adds two helpers:

  • find_merge_commit(pr_num, pr_events) prefers the closed event's commit (unchanged behavior, and GitHub's own authoritative link), and only when that is absent falls back to referenced events. Since a referenced event is raised by any commit merely mentioning the PR, each fallback candidate is confirmed against the Closes #N from line that merge_pr writes into every merge commit.
  • default_pick_branch(branch_names, already_picked) returns the highest-ranked release branch that has not already received the change. Backport mode previously defaulted the prompt to branch_names[0], which for a PR merged into branch-M.x is that very branch: accepting the default asked git to cherry-pick a commit onto the branch that already had it, which fails with The previous cherry-pick is now empty and lands the committer in the "Would you like to manually fix-up this merge?" prompt. The normal merge path already excluded target_ref from its defaults, so this consolidates both call sites onto the shared helper.

Why are the changes needed?

A PR opened against branch-4.x and merged there cannot currently be backported to branch-4.3 with the merge script at all -- the script's own API-close erases the trail that its backport mode depends on. Concretely, for #57713 (merged into branch-4.x as 881e5a94a15), re-running the script prints:

Start to merge pull request #57713
Pull request 57713 is not mergeable in its current form.
Continue? (experts only!) (y/N):

Answering y there would create a duplicate squash commit on branch-4.x; the cherry-pick prompt is never reached. The only recourse was a manual git cherry-pick -sx outside the tool, which also skips the merge comment the script would post. With this change the same invocation reaches:

Pull request 57713 has already been merged, assuming you want to backport
Found commit 881e5a94a15...
Enter a branch name [branch-4.3]:

Does this PR introduce any user-facing change?

No. dev/merge_spark_pr.py is a committer tool and is not part of any released artifact.

How was this patch tested?

Note: ruff could not be run in the authoring environment (no PyPI access); formatting follows the surrounding conventions in the file and is left to CI's lint job to confirm.

Was this patch authored or co-authored using generative AI tooling?

Generated-by: Claude Code (Opus 5)

@uros-b
[SPARK-58561][INFRA] Support backporting a PR merged into a non-defau…
29ecbf0 
…lt branch in `merge_spark_pr.py`

Detect an already-merged PR from `referenced` events when the `closed` event
carries no commit, so a PR merged into a non-default branch (e.g. one opened
against a rolling `branch-M.x`) can still be cherry-picked with the script.

Default the backport prompt to the highest branch that has not already received
the change, instead of the highest branch overall, which was the branch the PR
had just merged into and produced an empty cherry-pick.
@uros-b uros-b changed the title [SPARK-58561][INFRA] Support backporting a PR merged into a non-default branch in merge_spark_pr.py [SPARK-58561][INFRA] Support backporting a PR merged into a non-default branch Aug 4, 2026
cloud-fan
cloud-fan reviewed Aug 4, 2026
View reviewed changes

@cloud-fan cloud-fan left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

2 blocking, 0 non-blocking, 0 nits.
The intended backport flow has two correctness gaps: merge-commit identification can select an unrelated commit, and repeated invocations do not detect branches already backported.

Correctness (2)

  • dev/merge_spark_pr.py:463: The referenced-event marker can match copied PR-body text and select the wrong commit for backporting. -- see inline
  • dev/merge_spark_pr.py:1758: Repeated backport invocations default to a maintenance branch that already contains the change. -- see inline

Verification

I traced merge identification back to merge_pr: PR-body line starts are preserved in a git commit -m paragraph, so the fallback regex cannot identify the generated trailer by line anchoring alone. I also traced repeated backports: the closed-PR path performs one cherry_pick, exits, and on the next invocation rebuilds already_picked as only (target_ref,); it never checks which other release branches contain merge_hash.

PR description suggestions

  • Correct the claim that line anchoring distinguishes the generated trailer from copied PR-body text.
  • Document and handle repeated backport invocations: the current implementation does not discover maintenance branches that already contain the merge commit.

Comment thread dev/merge_spark_pr.py Outdated
Comment thread dev/merge_spark_pr.py Outdated
@uros-b
[SPARK-58561][INFRA][FOLLOWUP] Validate the merge footer and skip alr…
87c42a3 
…eady-backported branches

Address review feedback on the backport path:

- Identify a merge commit by the footer structure `merge_pr` generates (a "Closes"
  paragraph followed immediately by the authors paragraph) rather than by the
  "Closes #N from " line alone. `merge_pr` passes the PR body through as its own
  commit-message paragraph, so a body quoting another PR's closing line kept that
  line at the start of a line and satisfied the old check.

- Derive the already-picked set from the release branches that carry the merge
  footer, so a repeated invocation no longer defaults to a branch a previous run
  already backported to. `git branch --contains <merge_hash>` cannot see this: a
  cherry-pick is a new commit, and the footer is what `cherry-pick -x` copies, the
  same signal `dev/pr_merge_status.py` reads.

- Loop in backport mode so one invocation can reach several maintenance branches,
  as the normal merge path already does.
@uros-b
uros-b requested a review from cloud-fan August 4, 2026 15:11
uros-b
uros-b commented Aug 4, 2026
View reviewed changes

@uros-b uros-b left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @cloud-fan! PTAL again

cloud-fan
cloud-fan reviewed Aug 4, 2026
View reviewed changes

@cloud-fan cloud-fan left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1 addressed, 1 remaining, 2 new. (1 newly introduced, 1 late catch, 0 previously raised.)
3 blocking, 0 non-blocking, 0 nits.
The ordinary non-default-branch backport flow is improved, but merge-footer recognition and exhausted-branch selection still permit wrong or empty cherry-picks.

Remaining from prior review (1)

  • The new matcher still scans the entire copied PR body, so a body that quotes a complete merge footer (for example while discussing or reverting a commit) satisfies the Closes plus Authored-by regex. A referenced commit for a different PR can therefore still be selected as this PR's merge. Anchor the match to the final generated footer, or parse the final commit-message paragraphs, and add a regression case containing the complete quoted footer. -- existing thread

Correctness (2)

  • dev/merge_spark_pr.py:727: This branch scan reintroduces the same false-positive class that has_merge_footer is meant to avoid: git log --grep accepts the fragment anywhere in the commit message. A body merely quoting Closes #N from makes every containing release branch look already backported, so the script can skip the correct default and report a backport that never happened. Filter candidate commit messages with the validated footer matcher before mapping them to branches. -- see inline
  • dev/merge_spark_pr.py:771: When every known release branch is already in already_picked, this fallback selects branch_names[0], which is known to contain the change. Backport mode calls cherry_pick once before asking whether another pick is wanted, so rerunning after all branches have received the PR immediately offers an empty cherry-pick. Stop with an 'all branches already contain this change' message instead of returning an already-picked default. -- see inline

Verification

I traced generated commit messages from merge_pr through both referenced-event selection and release-branch discovery, including PR bodies that contain footer-shaped paragraphs. I also traced backport mode when every known release branch is already present in picked_refs; no tests were run as part of this review.

PR description suggestions

  • Correct the claim that a PR body cannot fake the complete footer structure; the body is copied verbatim and may quote an entire merge footer.
  • Document and cover the case where every known release branch already contains the change.

Comment thread dev/merge_spark_pr.py
Comment thread dev/merge_spark_pr.py Outdated
uros-b added 2 commits August 4, 2026 16:26
@uros-b
[SPARK-58561][INFRA][FOLLOWUP] Identify the merge footer by position …
0853820 
…and stop when no branch remains

Address the second round of review feedback:

- Identify the generated footer by position rather than structure. A PR body is
  copied verbatim into the merge commit, so it can quote another commit's entire
  footer, authors paragraph included; matching the structure anywhere in the
  message therefore still selected the wrong commit. `merge_pr` appends the footer
  last, so `merge_footer_pr` reads the final "Closes" paragraph and compares its
  number. Cherry-pick provenance lines may follow it, but no later "Closes"
  paragraph can.

- Validate the release-branch scan with the same matcher. `git log --grep` matches
  the fragment anywhere in a message, so a commit merely quoting the trailer made
  every containing branch look already backported; candidates are now confirmed
  before their branches count.

- Return None from `default_pick_branch` when every known branch already has the
  change, and have both call sites report that instead of defaulting to a branch
  whose cherry-pick would be empty.
@uros-b
[SPARK-58561][INFRA][FOLLOWUP] Apply ruff format to the exhausted-bra…
1f41a2d 
…nch message

`ruff format` collapses the string and its `%` operand onto one line, which fits
within the 100-character limit. Matches the sibling call site in the merge path.
cloud-fan
cloud-fan reviewed Aug 5, 2026
View reviewed changes

@cloud-fan cloud-fan left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

3 addressed, 0 remaining, 1 new. (0 newly introduced, 1 late catch, 0 previously raised.)
1 blocking, 0 non-blocking, 0 nits.
The prior correctness defects are addressed, but merge-status detection should be consolidated before this change lands.

Design / architecture (1)

  • dev/merge_spark_pr.py:730: Reuse one validated merge-status detector instead of adding a second branch/footer scan. dev/pr_merge_status.py already fetches official branches, finds commits carrying the PR trailer, and maps them to containing branches; this PR duplicates that flow while adding validation only here and omitting the analogue's fetch. Extract the common footer parsing and commit-to-branch discovery into a shared helper used by both scripts, with an explicit refresh policy, so the tools cannot disagree and future trailer changes have one implementation surface. -- see inline

Verification

I traced generated commit messages through fallback merge discovery and release-branch scanning, and compared that flow with dev/pr_merge_status.py. Both implementations locate trailer-bearing commits and map them to official remote branches. Their contracts diverge materially: only the PR's new implementation validates the final generated footer, while only the existing status script refreshes upstream tracking refs. No tests were run as part of this review.

Comment thread dev/merge_spark_pr.py
return pick_ref, pick_hash


def branches_with_merge_footer(pr_num, branch_names):

@cloud-fan cloud-fan Aug 5, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please share the existing merge-status detector instead of adding a second footer/branch scan here. dev/pr_merge_status.py already fetches official branches, finds commits carrying this trailer, and maps them to containing branches; this copy validates footer position but omits that refresh, while the existing copy refreshes but lacks this validation, so the two committer tools can disagree. Extract the footer parsing and commit-to-branch mapping into one helper used by both scripts, with an explicit refresh policy.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@cloud-fan cloud-fan cloud-fan left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@uros-b @cloud-fan